Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tarball all needed templates from different folder #214

Merged
merged 2 commits into from
Sep 19, 2024
Merged

Conversation

dachengx
Copy link
Collaborator

@dachengx dachengx commented Sep 19, 2024

as long as their names are different

Asking all templates are in the same folder does not make sense, configuration like: https://github.com/XENONnT/nton/blob/fcccf25d583e1ee218724c4962d6fac9583c5191/nton/wimp/configs/sr0_sr1/wimp_statistical_model_ac_optimization.yaml#L409 and https://github.com/XENONnT/nton/blob/fcccf25d583e1ee218724c4962d6fac9583c5191/nton/wimp/configs/sr0_sr1/wimp_statistical_model_ac_optimization.yaml#L553 should be allowed.

  1. Add a class LockableSet and initialize a variable TEMPLATE_RECORDS in alea/utils.py
  2. Update TEMPLATE_RECORDS in alea.utils._prefix_file_path, so TEMPLATE_RECORDS will record all needed templates absolute path
  3. Then we do not need alea.submitters.htcondor.SubmitterHTCondor._validate_template_path or alea.submitters.htcondor.SubmitterHTCondor._check_filename_unique any more
  4. In alea.submitters.htcondor.SubmitterHTCondor._tar_h5_files, first copy all templates into templates_tarball_dir then make tarball of templates_tarball_dir

@dachengx dachengx marked this pull request as ready for review September 19, 2024 04:24
@coveralls
Copy link

coveralls commented Sep 19, 2024

Pull Request Test Coverage Report for Build 10934406788

Details

  • 17 of 21 (80.95%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 89.51%

Changes Missing Coverage Covered Lines Changed/Added Lines %
alea/utils.py 17 21 80.95%
Totals Coverage Status
Change from base Build 10933551063: -0.2%
Covered Lines: 1681
Relevant Lines: 1878

💛 - Coveralls

@dachengx dachengx merged commit 0b7c580 into main Sep 19, 2024
7 checks passed
@dachengx dachengx deleted the tarball_templates branch September 19, 2024 04:29
Copy link
Collaborator

@hammannr hammannr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice solutionwith the lockable set! Looks good to me in general but I haven't tried it (yet). One small concern are the non-flat structures in the tar ball but I might be wrong here.
One question not entirely related to this PR: Shouldn't the toyfiles be included here as well? Since we no allow toydata mode "read" they should be tarred as well or am I missing something?

logger.warning(
"The template path contains subdirectories. All templates files will be tarred."
)

def _tar_h5_files(self, directory, template_tarball="templates.tar.gz"):
"""Tar all .h5 templates in the directory and its subdirectories into a tarball."""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring should be updated

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right.

alea/submitters/htcondor.py Show resolved Hide resolved
alea/submitters/htcondor.py Show resolved Hide resolved
for dirpath, dirnames, filenames in os.walk(self.template_path):
for filename in filenames:
logger.info(f"File: {filename} in {dirpath}")
if self._contains_subdirectories(self.template_path):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_contains_subdirectories is no longer used after this PR sop we might as well delete it or am I missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right.

@dachengx
Copy link
Collaborator Author

dachengx commented Sep 19, 2024

Really nice solutionwith the lockable set! Looks good to me in general but I haven't tried it (yet). One small concern are the non-flat structures in the tar ball but I might be wrong here. One question not entirely related to this PR: Shouldn't the toyfiles be included here as well? Since we no allow toydata mode "read" they should be tarred as well or am I missing something?

Toydata is added to the file in this line:

if args_dict["toydata_mode"] == "read":
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants